GPU Framework: automatically generate GPUDefParametersDefaults.h #14972
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
cima22
force-pushed
the
json_param_definitions
branch
2 times, most recently
from
71b8e58 to
16e7fec
Compare
cima22
force-pushed
the
json_param_definitions
branch
2 times, most recently
from
7ae23b1 to
174fcd4
Compare
davidrohr
left a comment
davidrohr
left a comment
There was a problem hiding this comment.
Looks good in general, but perhaps you could implement some improvements.
dependencies/FindO2GPU.cmake
Outdated
| elseif(backend STREQUAL "HIP") # HIP filter | ||
| set(TARGET_ARCH "${HIP_TARGET}" PARENT_SCOPE) | ||
| return() | ||
| else() # Return both |
There was a problem hiding this comment.
Why do you return both?
I would probably have a generic default target, which can be queried by "backend = default" explicitly, we could also set a default for opencl explicitly, and in case the backend does not match any known one, I would fail with message(FATAL_ERROR ...)
There was a problem hiding this comment.
I return both s.t. the default parameters header has always both a CUDA or HIP fallback in case both backends are required to be compiled
There was a problem hiding this comment.
ok, now I think I understood how it works :).
Then I would call it "ALL", and export explicitly the defaults for all backends, including OpenCL.
And still fail in an else case for unknown backends.
dependencies/FindO2GPU.cmake
Outdated
| function(set_target_cuda_arch target) | ||
| if(CUDA_COMPUTETARGET AND (CUDA_COMPUTETARGET MATCHES "86" OR CUDA_COMPUTETARGET MATCHES "89")) | ||
| function(detect_gpu_arch backend) # Detect GPU architecture, optionally filterring by backend | ||
| set(TARGET_ARCH "") |
There was a problem hiding this comment.
Since your if / else below covers all cases, I think there is no reason to initialize them to empty?
| else() | ||
| set(TARGET_ARCH ${GPU_ARCH}) | ||
| endif() | ||
| add_custom_command( |
There was a problem hiding this comment.
I do not understand, why you need to execute CMake explicitly on that cmake file [gpu_param_header_generator.cmake](https://github.com/AliceO2Group/AliceO2/pull/14972/changes#diff-69d48e46e923fe79aeab700a7a1ea6b0dc5b8d927a8493d8f449b168b52d50ba).
Couldn't you just include that cmake file, that would also execute it, or do I miss something?
Or better, I would put everything in that CMake file in a function, include that CMake file, and then just call the function.
| file(APPEND "${TMP_HEADER}" "\n// Default parameters if not defined for the target architecture\n\n") | ||
| #Default parameters | ||
| foreach(TYPE IN LISTS TYPES) | ||
| # Get all keys of this TYPE as a semicolon-separated list |
There was a problem hiding this comment.
I don't like the copy&paste from the code above. Could you extract the duplicated part in a function?
…nerate GPU parameters using json file and CMake
cima22
force-pushed
the
json_param_definitions
branch
from
174fcd4 to
a97a95b
Compare
cima22
force-pushed
the
json_param_definitions
branch
from
a97a95b to
b3b1c57
Compare
2 participants
Remove GPUDefParametersDefaults.h and generate GPU parameters using json file and CMake. Results checked with deterministic mode.